Guard BadImport's type-annotation move against unpositioned trees - #6083
Open
adityaanikam wants to merge 1 commit into
Open
Guard BadImport's type-annotation move against unpositioned trees#6083adityaanikam wants to merge 1 commit into
adityaanikam wants to merge 1 commit into
Conversation
BadImport.moveTypeAnnotations() unconditionally does builder.delete(annotation) for every TYPE_USE-targeted annotation on the qualified type, then reinserts it before the qualified type. That delete requires a valid start/end source position for the annotation tree. A record component's type-use annotation can be reached through javac's synthesized members (the desugared field/accessor/canonical constructor), whose copy of the annotation tree has no recorded end position. Building a Replacement from that position throws SourcePositionException (IllegalArgumentException before 2.48.0), aborting the whole compilation -- BadImport is on by default, so no special configuration is needed to hit it. Skip the move for annotations without an explicit source position, using ASTHelpers.hasExplicitSource() (the same helper already used elsewhere in the codebase for this exact check). The identifier is still qualified by the prefixWith() call in the caller; only the annotation relocation is skipped, so the fix stays useful instead of being dropped entirely. Fixes google#6074
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Author
|
@googlebot I signed it! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6074
Problem
BadImport.moveTypeAnnotations()unconditionally callsbuilder.delete(annotation)for everyTYPE_USE-targeted annotation on a qualified type, then reinserts it before the qualified identifier. That delete requires a valid start/end source position for the annotation tree.A record component's type-use annotation can be reached through javac's synthesized members (the desugared field/accessor/canonical constructor), whose copy of the annotation tree has no recorded end position. Building a
Replacementfrom that position throwsSourcePositionException(IllegalArgumentExceptionbefore 2.48.0), aborting the whole compilation.BadImportis on by default, so this needs no special configuration to hit -- any record component whose type is an imported nested class, annotated with something carryingTYPE_USEin its@Target(e.g. Bean Validation's@NotNull), aborts the build.Fix
Skip the move for annotations without an explicit source position, using
ASTHelpers.hasExplicitSource()-- the same helper this codebase already uses elsewhere for exactly this check. The identifier is still qualified by theprefixWith()call in the caller; only the annotation relocation is skipped, so the fix stays useful instead of being dropped entirely.Testing
Added
BadImportTest#recordComponentWithTypeUseAnnotation_doesNotCrash, reproducing the record-component +TYPE_USE-annotation shape from the issue, adjacent to the existingnestedTypeUseAnnotationtest this mirrors.Honest caveat on local verification: I could not get a local Windows build past
error_prone_check_api--ErrorProneSignatureGenerator.java(a file this PR does not touch) fails withan enclosing instance that contains Types.SignatureGenerator is requiredwhenmaven-compiler-pluginforks ajavacsubprocess, on both the exact JDK 25 GA build (25+36) and a later25.0.4patch, across two vendors (Temurin, Zulu). This reproduces on unmodifiedmastertoo, so it's unrelated to this change -- and upstream CI is green on the commit I branched from (9fefe418), so it appears to be a Windows-specific forked-javacquirk on my machine rather than a real break. I was not able to run the new test locally as a result. The expected output in the added test is reasoned from the existingnestedTypeUseAnnotationtest's behavior, not independently confirmed by a passing local run -- CI will be the first real signal on it, and I'll fix it promptly if it's wrong.